-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Also ignore subclasses of data_sync_excluded_exceptions
#165
Conversation
0bc1d8a
to
2f143f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - thanks for fixing so quickly
I wonder if we can test this branch out directly in Email Alert API or similar to verify it works for real before doing a release? Just would seem a shame if we hit a different problem and had to re-release gem again.
@@ -25,8 +25,13 @@ def should_capture=(closure) | |||
def ignore_excluded_exceptions_in_data_sync | |||
lambda { |error_or_event| | |||
data_sync_ignored_error = data_sync_excluded_exceptions.any? do |exception_to_ignore| | |||
exception_to_ignore = Object.const_get(exception_to_ignore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For compatibility with the Raven interface this should be able to handle the actual class passed in. https://github.com/getsentry/sentry-ruby/blob/7a0cebc54c5508d379769c7af688c8218721d734/lib/raven/configuration.rb#L447
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was quite simple to add, so I've tacked a commit on - but I do wonder how necessary it is, given data_sync_excluded_exceptions
is our own construct, and the Raven docs seem to suggest passing string values to excluded_exceptions
(the closest thing to what we've built).
Happy to remove the commit I've just added, or keep it - I don't feel strongly either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool thanks - I think it's better that the APIs are compatible. It probably isn't absolutely necessary but does seem a common pattern where classes can be passed in directly or as a string, feels unusual to only accept string but not the actual constant.
We're seeing PG:UndefinedTable errors in Sentry, which we'd like to ignore during the data sync. By default we're ignoring PG::Error exceptions already, but this is a specific sub class of PG::Error (and there are many more we envisage wanting to ignore). We could list all of these subclasses in the `data_sync_excluded_exceptions` array, but what we'd rather do is describe the parent class and have any of its children exceptions ignored. `PG::Error === PG::UndefinedTable.new` is true, whereas `"PG::Error" === PG::UndefinedTable.new.class` is false. This commit makes the default `should_capture` behaviour dig deeper into the exception, checking that the exception type is classed as any one of the errors we've chosen to ignore. From now on, any exceptions that are a subclass of `PG::Error` will be ignored during the data sync. https://trello.com/c/rAlvGlSp/2123-5-ignore-data-sync-errors-in-govukappconfig
All of the tests were testing scenarios that occur during a data sync. A context is the best vehicle for that.
This maintains parity with the Raven interface: https://github.com/getsentry/sentry-ruby/blob/7a0cebc54c5508d379769c7af688c8218721d734/lib/raven/configuration.rb#L447
2f143f0
to
4130f3d
Compare
Thanks for the review, @kevindew. The only way I can think of testing it would be to make a branch of email-alert-api which pulls in this branch instead of the govuk_app_config gem, building that to integration, and waiting for the data sync to happen overnight (hoping nobody builds email-alert-api in the meantime). Is that what you suggest? Alternatively I guess I could edit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks @ChrisBAshton
With regards to testing we could always merge Email Alert API to be pointing at the git source rather than a Rubygems release for overnight testing - less brittle than a branch deploy.
But I think the env var idea is better, you shouldn't need to do the whole data sync though you just need something that'll generate a PG::Error
for example: ActiveRecord::Base.connection.execute("SELECT * FROM blah")
Tested on integration with your help (
|
We're seeing PG:UndefinedTable errors in Sentry, which we'd like
to ignore during the data sync. By default we're ignoring PG::Error
exceptions already, but this is a specific sub class of PG::Error
(and there are many more we envisage wanting to ignore). We
could list all of these subclasses in the
data_sync_excluded_exceptions
array, but what we'd rather dois describe the parent class and have any of its children
exceptions ignored.
PG::Error === PG::UndefinedTable.new
is true, whereas"PG::Error" === PG::UndefinedTable.new.class
is false.This commit makes the default
should_capture
behaviour digdeeper into the exception, checking that the exception type
is classed as any one of the errors we've chosen to ignore.
From now on, any exceptions that are a subclass of
PG::Error
will be ignored during the data sync.
https://trello.com/c/rAlvGlSp/2123-5-ignore-data-sync-errors-in-govukappconfig